Skip to content

ERR non-ISO formats don't show position of error #50366

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

MarcoGorelli
Copy link
Member

@MarcoGorelli MarcoGorelli commented Dec 20, 2022


@MarcoGorelli MarcoGorelli changed the title Note position in err message ERR non-ISO formats don't show position of error Dec 20, 2022
@MarcoGorelli MarcoGorelli marked this pull request as draft December 20, 2022 21:18
@MarcoGorelli MarcoGorelli force-pushed the note-position-in-err-message branch from 464aa84 to a2cff0b Compare December 20, 2022 21:35
@MarcoGorelli MarcoGorelli marked this pull request as ready for review December 20, 2022 21:35
@MarcoGorelli MarcoGorelli force-pushed the note-position-in-err-message branch from a2cff0b to 303a648 Compare December 20, 2022 21:58
@MarcoGorelli MarcoGorelli added Error Reporting Incorrect or improved errors from pandas Datetime Datetime data dtype labels Dec 20, 2022
Comment on lines 399 to 400
except (ValueError, OutOfBoundsDatetime) as ex:
ex.args = (str(ex) + f" at position {i}", )
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar to

except (ValueError, OverflowError) as ex:
ex.args = (f"{ex} present at position {i}", )

I just removed the word 'present' as I don't think sounds good with the errors from this file

i.e.

time data ' ' does not match format '%m/%d/%Y' \(match\) at position 2

sounds better than

time data ' ' does not match format '%m/%d/%Y' \(match\) present at position 2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit but why not use a full f-string instead of addition between 2 strings here?

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So IIUC the message is a bit different between ISO / non-ISO (the former has the word present?). Can we align the messages?

Comment on lines 399 to 400
except (ValueError, OutOfBoundsDatetime) as ex:
ex.args = (str(ex) + f" at position {i}", )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit but why not use a full f-string instead of addition between 2 strings here?

@MarcoGorelli MarcoGorelli marked this pull request as draft December 20, 2022 23:18
@MarcoGorelli MarcoGorelli marked this pull request as ready for review December 21, 2022 10:34
)
with pytest.raises(OutOfBoundsDatetime, match=msg):
with tm.assert_produces_warning(warning, match="Could not infer format"):
to_datetime("2417-10-27 00:00:00", format=format)
to_datetime("2417-10-10 00:00:00", format=format)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slightly changing the input here so we can parametrise of it with both "%Y-%m-%d %H:%M:%S" and "%Y-%d-%m %H:%M:%S" (to check ISO vs non-ISO)

@MarcoGorelli
Copy link
Member Author

MarcoGorelli commented Dec 21, 2022

I've aligned them now, thanks - this should make the diff in #50242 much smaller


In [1]: to_datetime(['2020-01-01', '9999-01-01'], format='%Y-%m-%d')
---------------------------------------------------------------------------
OutOfBoundsDatetime: Out of bounds nanosecond timestamp: 9999-01-01 00:00:00 present at position 1

In [2]: to_datetime(['2020-01-01', '9999-01-01'], format='%Y-%d-%m')
---------------------------------------------------------------------------
OutOfBoundsDatetime: Out of bounds nanosecond timestamp: 9999-01-01 00:00:00 present at position 1

In [3]: to_datetime(['2020-01-01', 'foo'], format='%Y-%m-%d')
---------------------------------------------------------------------------
ValueError: time data "foo" at position 1 doesn't match format "%Y-%m-%d" (match)

In [4]: to_datetime(['2020-01-01', 'foo'], format='%Y-%d-%m')
----------------------------------------------------------------------------
ValueError: time data "foo" at position 1 doesn't match format "%Y-%d-%m" (match)

@MarcoGorelli MarcoGorelli requested a review from WillAyd December 21, 2022 10:42
@@ -1133,10 +1139,10 @@ def test_datetime_invalid_scalar(self, value, format, warning):
assert res is NaT

msg = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick can you use msg = "|".join(...) instead of a single string

raise ValueError(
f"time data \"{val}\" at position {i} doesn't "
f"match format \"{format}\""
f"match format \"{format}\" ({match_msg})"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ending this with "search" seems weird (and i dont see it in any test cases)

Copy link
Member Author

@MarcoGorelli MarcoGorelli Dec 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's true that it wasn't tested, but it's been like this since pandas 0.24:

In [1]: import pandas

In [2]: pandas.__version__
Out[2]: '0.24.0'

In [3]: pandas.to_datetime(['2020-01-f00'], format='%Y-%d-%m', exact=False)
---------------------------------------------------------------------------

ValueError: time data '2020-01-f00' does not match format '%Y-%d-%m' (search)

Maybe we can just remove it - people typically know whether they've passed exact, and it doesn't really add much

Especially as it was never there for (more common) ISO formats:

In [4]: pandas.to_datetime(['2020-01-f00'], format='%Y-%m-%d', exact=False)
---------------------------------------------------------------------------
ValueError: time data 2020-01-f00 doesn't match format specified

Let's just unify them to

time data '2020-01-f00' does not match format '%Y-%d-%m'

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks pretty good

@@ -396,7 +401,15 @@ def array_strptime(

result_timezone[i] = tz

except (ValueError, OutOfBoundsDatetime):
except OutOfBoundsDatetime as ex:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than breaking off this branch can we not keep the existing one and just specialize for the OOB exception within?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, that's better - thanks!

Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome thanks @MarcoGorelli

@WillAyd
Copy link
Member

WillAyd commented Dec 27, 2022

Looks like a merge conflict but feel free to merge after that

@MarcoGorelli
Copy link
Member Author

cool, thanks for reviewing!

@@ -1882,7 +1891,10 @@ def test_dataframe_mixed(self, cache):
def test_dataframe_float(self, cache):
# float
df = DataFrame({"year": [2000, 2001], "month": [1.5, 1], "day": [1, 1]})
msg = "cannot assemble the datetimes: unconverted data remains: 1"
msg = (
r"^cannot assemble the datetimes: unconverted data remains at position "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NBD but is the "r" necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be, it's just a habit to always use that in regular expressions

@MarcoGorelli MarcoGorelli added this to the 2.0 milestone Dec 28, 2022
@MarcoGorelli MarcoGorelli merged commit 4e88c3f into pandas-dev:main Dec 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype Error Reporting Incorrect or improved errors from pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERR non-ISO formats don't show position of error
4 participants